Hi @sushant-me , thank you for your contribution. We appreciate you taking the time to submit this pull request. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you.

One scoping note on the residual, so the trade-off is on the record — the same class of note as the one on the Go port, but the reasoning differs here and I did not want to copy it across.

This guard closes the path where a remote MCP server advertises a reserved name. It does not change the underlying property that an in-model tool never occupies its name in the tool map:

• GoogleSearchTool.processLlmRequest (and the other in-model tools) only append to configBuilder.tools(...); they never put the name into llmRequestBuilder.tools().
• Nothing else does either — I could not find a duplicate-name check at the tools() map level in the flows (the only duplicate check is for sub-agent names in BaseAgent).

So an in-process tool registered under google_search — a FunctionTool or an AgentTool — is accepted alongside the in-model tool. In this repository that requires the application author to pick the name, so I treated it as a footgun rather than the adversarial case and kept this PR to the server-controlled path.

Where Java differs from Go: in adk-go the equivalent map is map[string]any and consumers type-assert to tool.Tool, so occupying a name with a sentinel breaks them — that is why I left the general fix alone there. Here LlmRequest.tools() is already Map<String, BaseTool> (LlmRequest.java:89), so registering a sentinel or a real holder under the in-model name is type-safe. The open question is only whether a sentinel value is acceptable to advertise, which is a design decision rather than a typing constraint.

Happy to prepare that broader change if you would prefer the invariant enforced in one place instead of at each boundary — it touches high-fan-in classes, so I did not want to fold it in unasked.

Thanks, @hemasekhar-p — appreciated.

One thing that may help while it is in review: RESERVED_TOOL_NAMES currently carries all 13 names Java puts on the wire, and I checked it is neither short nor over-broad — every entry is a super("...") literal in a non-test source, plus set_model_response and transfer_to_agent which the framework contributes outside a tool class. Two names an earlier revision of my notes carried over from the Go port (finish_task, task_completed) are not Java names and are deliberately absent.

The two halves of #1513 fail differently in this code, which is why the guard covers both:

• In-model built-ins (google_search, google_maps, url_context, vertex_ai_search, code_execution) append only to config.Tools and never reach appendTools, so a callable tool takes the name and Functions.handleFunctionCalls resolves it to the server tool.
• Callables (set_model_response and friends) are already fail-closed — appendTools throws Duplicate tool name — so there the guard converts a run-aborting exception into a clean registration error.

Happy to adjust the list, the error type, or the placement if your team prefers a different shape.

Done — thank you for reading the control flow closely enough to find this. The check now runs after selection, and there is a test for the case you described.

What changed

The reserved-name check sat inside the mapping step, so it ran before isToolSelected:

.map(tool -> {
  if (RESERVED_TOOL_NAMES.contains(tool.name())) throw new IllegalArgumentException(...);
  return new McpTool(tool, ...);
})
.filter(tool -> isToolSelected(tool, toolFilter, readonlyContext)));

It now runs in the flow, after selection:

.map(tool -> new McpTool(tool, this.mcpSession, this.mcpSessionManager, this.objectMapper))
.filter(tool -> isToolSelected(tool, toolFilter, readonlyContext))
.map(tool -> {
  if (RESERVED_TOOL_NAMES.contains(tool.name())) throw new IllegalArgumentException(...);
  return tool;
});

One note on the ordering, in case it looks arbitrary: the McpTool has to be constructed before the filter rather than after it, because isToolSelected takes a BaseTool and dispatches on either a ToolPredicate or a list of names. So "wrap → filter → check" is the only order that keeps the existing filter semantics.

The test

getTools_toolFilterExcludesReservedName_loadsSelectedToolsInsteadOfFailing — the server advertises a reserved name plus tool1, and the caller's toolFilter is ["tool1"]. It asserts the toolset yields exactly ["tool1"].

I ran it against the old ordering, because an assertion that has never failed is not evidence:

OLD ordering:
  McpToolsetTest…:399 » McpToolLoading
  Invalid argument encountered during tool loading.        <- your bug, reproduced
NEW ordering:
  Tests run: 27, Failures: 0, Errors: 0

getTools_refusesReservedToolName still passes unchanged, so a reserved name that is selected is still a fatal, non-retried error. The guard still guards — it just no longer over-fires.

Checks: ./mvnw -pl core test -Dtest=McpToolsetTest → 27 tests, 0 failures. google-java-format clean.

One question, since you know the code better than I do: is "after selection" the placement you had in mind, or would you rather the check live inside isToolSelected itself so every toolset subclass inherits it? I put it in getTools because the error is meant to be fatal and non-retried, and that is expressed by where it sits relative to retryWhen — but if you would prefer it owned by the base class, that is a different and probably better factoring, and I would rather do it your way than argue for mine.

I need to correct my previous comment. I wrote "Done", but what I pushed is not the rework you asked for, and your two objections are both still true of the current head.

You asked for the error to move to BaseLlmFlow.getRequestProcessorFromTools, so that it fires only on a real name collision and covers McpAsyncToolset, FunctionTool and AgentTool through the one path every tool passes through.

What 5635996e actually did was keep the check inside McpToolset and change when it runs — after the tool filter rather than before. That is a narrower change than it sounds:

• Your first objection stands. McpToolset still cannot see the agent's other tools, so it still rejects a server whose tool merely shares a name with an in-model tool the agent does not even have. An agent with no GoogleSearchTool still cannot load a server exposing google_search.
• Your second objection stands. McpAsyncToolset is still unchecked; initTools() wraps every server tool without a check, exactly as MCP toolset: in-model built-ins (e.g. google_search) can be shadowed by a server tool; a server tool named set_model_response aborts the run #1513 describes.

So moving the check later within McpToolset did not address the reason you asked for the move. I read my own change as satisfying the review when it only softened the symptom, and "Done" was the wrong word for it.

I have read getRequestProcessorFromTools and LlmRequest.Builder.appendTools (the Duplicate tool name throw at LlmRequest.java:219), and I understand the rule you want: after the processors have run and the request is built, fail when a tool with no function declaration shares a name with a function tool in the request — and only in that direction, so two ExampleTools with the default name keep working.

What I have not yet worked out is how to read "a tool with no function declaration" off the built request reliably, which is the part the check depends on. I would rather say that plainly than push a second attempt that reads as complete. This is not done, and I will rework it in the direction you described, with the test that exercises the in-model tool both before and after the clashing tool, and with the PR description updated to note that an agent carrying both GoogleSearchTool and its own google_search function would begin to fail.

If you would prefer to close this while I do that, that is reasonable — nothing here is worth leaving in a state that looks answered when it is not.

I said the open question was how to read "a tool with no function declaration" off the built request. I have worked that out, and the answer changes where the check can live. Recording it so the rework has a concrete route.

A declaration-less tool never enters LlmRequest.tools(). BaseTool.processLlmRequest returns before the only appendTools call:

public Completable processLlmRequest(LlmRequest.Builder b, ToolContext ctx) {
  if (declaration().isEmpty()) {
    return Completable.complete();          // returns here
  }
  b.appendTools(ImmutableList.of(this));    // never reached

And the in-model tools override processLlmRequest to write to the config tools instead. Verified by counting appendTools calls in each:

GoogleSearchTool adds Tool.builder().googleSearch(...) to GenerateContentConfig.tools() and returns, which is exactly the "second tool with the same name" you described — it is in the config, not in the map.

So the comparison the check needs cannot be made from the built LlmRequest alone: the declaration-less half is not in it. It has to read the tool list from the agent. In getRequestProcessorFromTools both halves are reachable — agent.toolsUnion() (plus each baseToolset.getTools(readonlyContext)) gives the declaration-less names via declaration().isEmpty(), and builder.build().tools() gives the function-tool names. Fail on the intersection, in that direction only, so two ExampleTools with the default name still work.

One thing worth knowing before it is written: an in-model tool's presence in toolsUnion() does not mean its declaration was added to the request. GoogleSearchTool.processLlmRequest errors for a non-gemini- model, so "the agent declares it" and "the request carries it" can differ. Whether the check should key on the agent's list or on the config tools the request actually ended up with is a judgement call I would rather you make than guess at — the first is simpler, the second is closer to what the model sees.

I am not pushing this yet, and I want to be explicit about why rather than silent: there is no gradlew in my checkout and no Gradle on PATH, so I cannot compile or run a test for core/ here. I have already put one unverified change in front of you on this PR and labelled it done when it was not; I would rather hand you the analysis and a working build than do that twice.

If you can point me at how you run the module's tests, or if it is easier for you to take it from here, either is fine — I will rework it against a tree I can actually build.

Reworked in 9c0ae01b, in the direction you described, and I think your reading of the mechanism was the right one — following it is what made the shape obvious.

Where it now lives. The error is in BaseLlmFlow.getRequestProcessorFromTools, so it fires only on a real collision and covers McpAsyncToolset, FunctionTool and AgentTool through the one path. The McpToolset list is gone.

Detecting the collision needed one thing I had to check first. A declaration-less tool never enters LlmRequest.tools() at all — BaseTool.processLlmRequest returns before the only appendTools call:

if (declaration().isEmpty()) {
  return Completable.complete();          // returns here
}
b.appendTools(ImmutableList.of(this));    // never reached

and the in-model tools override it to write into the request config instead (GoogleSearchTool, UrlContextTool, LoadArtifactsTool, ListSkillsTool each call appendTools zero times). So the declaration-less half cannot be read off the built request. The check reads it from agent.toolsUnion() plus each baseToolset.getTools(...), filtered on declaration().isEmpty(), and intersects with the built request's tools() map.

Only that direction is rejected, so two declaration-less tools may still share a name and default-named ExampleTools keep working.

Tests. The collision case runs the in-model tool both before and after the clashing function tool, as you asked, plus a negative case for the both-declaration-less setup. I confirmed each new test fails with the guard reverted:

expected java.lang.IllegalArgumentException to be thrown, but nothing was thrown

Full core suite: 1856 tests, 0 failures. Built and run with ./mvnw on JDK 25; CI pins 17, and I have not run it under 17.

PR description now carries the revision history and the note you asked for: an agent with both GoogleSearchTool and its own google_search function will start failing, and the fix is to rename one.

One correction to my earlier comment on this thread: I said I could not build core because there was no Gradle wrapper. That was wrong — this repo uses Maven, ./mvnw is in the tree, and it works. My apology for the detour; the rework would have been done a round earlier if I had checked which build system the repo actually uses.

Rebased onto current main and re-verified, since the branch had drifted 1286 commits behind and this change sits in a high-fan-in file that upstream has been editing (BaseLlmFlow.java moved 205 lines in the meantime).

The rebase was clean — no conflicts — and my change is unchanged in substance: still BaseLlmFlow.getRequestProcessorFromTools, still 2 files, +107/-0.

Re-ran everything against the new base rather than assuming the old results carried over:

core: 1997 tests, 0 failures, 0 errors, 0 skipped        (./mvnw -pl core test)
BaseLlmFlowTest: 29 tests, 0 failures

And the revert-check again, on the new base, to confirm the test still pins the behaviour rather than passing because the code happens to line up:

[ERROR] ...rejectsDeclarationlessNameCollision:989 expected
        java.lang.IllegalArgumentException to be thrown, but nothing was thrown

One thing I want to be explicit about: the earlier +316/-1 figure included the McpToolset guard. That is gone — McpToolset.java and McpToolsetTest.java are byte-identical to main now. The +107/-0 is the whole change.